VPLAY-12213: ABR Refactoring part 3 - encapsulate remaining abr logic #901

nu641001 · 2026-01-23T13:05:21Z

Reason for change: To encapsulate all ABR related logic inside abr.cpp and abr.h. This will help in future to add/remove different bandwidth estimation algorithms without affecting other parts of the code.
Risks: Low
Test Procedure: Test with Linear and VOD
Priority: P1

Copilot

Pull request overview

This pull request refactors ABR (Adaptive Bitrate) logic by encapsulating bandwidth estimation into a pluggable architecture. The changes move bandwidth tracking from PrivateInstanceAAMP into ABRManager and introduce a BandwidthEstimatorBase interface with two implementations: RMOBandwidthEstimator (legacy algorithm) and HarmonicEWMAEstimator (new algorithm).

Changes:

Introduced BandwidthEstimatorBase interface and two concrete implementations for bandwidth estimation algorithms
Moved bandwidth state management from PrivateInstanceAAMP to ABRManager
Updated all callers to use ABRManager methods instead of direct PrivateInstanceAAMP methods
Added configuration option to select bandwidth estimator type
Updated tests and fakes to reflect the new architecture

Reviewed changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated 15 comments.

Show a summary per file

File	Description
abr/BandwidthEstimatorBase.h	New base interface for bandwidth estimator implementations
abr/RMOBandwidthEstimator.h/cpp	Legacy Rolling Median Outlier bandwidth estimation algorithm extracted from ABR code
abr/HarmonicEWMAEstimator.h/cpp	New Harmonic EWMA bandwidth estimation algorithm (renamed from NetworkBandwidthEstimator)
abr/abr.h/cpp	Extended ABRManager with bandwidth estimator management and state tracking
priv_aamp.h/cpp	Removed bandwidth state members and delegated to ABRManager
streamabstraction.cpp	Updated to call ABRManager methods; added persist bandwidth logic
fragmentcollector_hls.cpp/mpd.cpp	Updated to use ABRManager bandwidth methods
AampDefine.h/AampConfig.h/cpp	Added BandwidthEstimatorType enum and configuration option
test/utests/tests/*/CMakeLists.txt	Added new source files to test builds
test/utests/tests/PrivAampTests/PrivAampTests.cpp	Updated tests to use ABRManager interface
test/utests/tests/NetworkBandwidthEstimator/	Renamed to HarmonicEWMAEstimator; added progress update test
test/utests/fakes/	Updated fakes to remove deprecated methods

abr/abr.cpp

priv_aamp.cpp

streamabstraction.cpp

abr/abr.cpp

test/utests/tests/PrivAampTests/PrivAampTests.cpp

Copilot · 2026-01-23T13:13:23Z

streamabstraction.cpp

+			if(ISCONFIGSET(eAAMPConfig_PersistLowNetworkBandwidth)||ISCONFIGSET(eAAMPConfig_PersistHighNetworkBandwidth))
+			{
+				if(networkBandwidth > 0 && aamp->mbPlayEnabled)
+				{
+					ABRManager::setPersistBandwidth(networkBandwidth);
+					ABRManager::mPersistBandwidthUpdatedTime = aamp_GetCurrentTimeMS();
+				}
+			}


This persist bandwidth logic is duplicated in multiple places within streamabstraction.cpp (lines 2585-2592, 2703-2710, 2959-2966, 2997-3004). The exact same code block appears four times. This violates the DRY principle and makes maintenance difficult. Consider extracting this logic into a helper method in ABRManager (e.g., UpdatePersistBandwidthIfNeeded) that can be called from these locations.

Copilot · 2026-01-23T13:13:23Z

streamabstraction.cpp

+			if(ISCONFIGSET(eAAMPConfig_PersistLowNetworkBandwidth)||ISCONFIGSET(eAAMPConfig_PersistHighNetworkBandwidth))
+			{
+				if(stAbrInfo.networkBandwidth > 0 && aamp->mbPlayEnabled)
+				{
+					ABRManager::setPersistBandwidth(stAbrInfo.networkBandwidth);
+					ABRManager::mPersistBandwidthUpdatedTime = aamp_GetCurrentTimeMS();
+				}
+			}


The persist bandwidth logic is also duplicated here. This is the same code as appears on lines 2585-2592. Consider consolidating this duplication by extracting it into a helper method.

abr/abr.cpp

… inside abr.cpp Reason for change: To encapsulate all ABR related logic inside abr.cpp and abr.h. This will help in future to add/remove different bandwidth estimation algorithms without affecting other parts of the code. Risks: Low Test Procedure: Test with Linear and VOD Priority: P1 Signed-off-by: Nandakishor U M <nu641001@gmail.com>

Copilot

Pull request overview

Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.

Copilot · 2026-01-23T23:40:27Z

abr/abr.h

 #include <mutex>
 #include "AampMediaType.h"
+#include "BandwidthEstimatorBase.h"
+class BandwidthEstimatorBase;


Redundant forward declaration. Line 33 contains a forward declaration of BandwidthEstimatorBase, but line 32 already includes the full header file BandwidthEstimatorBase.h. The forward declaration should be removed as it's unnecessary and could cause confusion.

Suggested change

class BandwidthEstimatorBase;

Copilot · 2026-01-23T23:40:28Z

abr/abr.cpp

+void ABRManager::ResetCurrentlyAvailableBandwidth()
+{
+	std::lock_guard<std::mutex> lock(mBandwidthEstimatorLock);
+	mBandwidthEstimator->ResetCurrentlyAvailableBandwidth();


Potential null pointer dereference. The function calls mBandwidthEstimator->ResetCurrentlyAvailableBandwidth() without checking if mBandwidthEstimator is null. While the estimator is initialized in the constructor, there could be edge cases where it's null. Additionally, HarmonicEWMAEstimator doesn't override ResetCurrentlyAvailableBandwidth(), which means when using that estimator, this call will invoke the base class's no-op implementation and fail to actually reset the bandwidth state. Consider adding a null check and ensuring HarmonicEWMAEstimator properly implements this method.

Suggested change

mBandwidthEstimator->ResetCurrentlyAvailableBandwidth();

if (mBandwidthEstimator)

{

mBandwidthEstimator->ResetCurrentlyAvailableBandwidth();

}

Copilot · 2026-01-23T23:40:28Z

abr/abr.cpp

+			BandwidthEstimatorConfig config;
+			config.mAbrCacheLife = eAAMPAbrConfig.abrCacheLife;
+			config.mAbrCacheLength = eAAMPAbrConfig.abrCacheLength;
+			config.mAbrCacheOutlier = eAAMPAbrConfig.abrCacheOutlier;
+			config.mLowLatencyCacheLength = DEFAULT_ABR_CHUNK_CACHE_LENGTH;
+			mBandwidthEstimator.reset(new RMOBandwidthEstimator(config));


Configuration initialization issue. When SetBandwidthEstimatorType is called from the constructor (line 59), it accesses eAAMPAbrConfig members (abrCacheLife, abrCacheLength, abrCacheOutlier) that haven't been initialized yet, as ReadPlayerConfig() hasn't been called. These will all be zero due to default initialization. This means the RMOBandwidthEstimator will be created with all-zero configuration values initially, which may not represent the correct default behavior. Consider either: 1) Using explicit default constants instead of relying on eAAMPAbrConfig in the constructor path, or 2) Deferring estimator creation until after configuration is loaded.

Copilot · 2026-01-23T23:40:28Z

abr/HarmonicEWMAEstimator.h

+public:
+	HarmonicEWMAEstimator() = default;
+
+	const char *GetNetworkEstimatorName() const override;
+	void Reset() override;
+	void AddBandwidthSample(BitsPerSecond downloadbps, bool lowLatencyMode) override;
+	void UpdateDownloadMetrics(const DownloadMetrics &curl) override;
+	void UpdateDownloadProgress(const DownloadProgressInfo &progressInfo) override;
+	BitsPerSecond GetBandwidthBitsPerSecond() override;
+	double GetThroughputBytesPerSecond() override;
+	double GetTimeToFirstByteSeconds() override;
+	double GetPredictedDownloadTimeSeconds(std::size_t segment_size_bytes) override;
+};


Missing override of ResetCurrentlyAvailableBandwidth(). The HarmonicEWMAEstimator class doesn't override the ResetCurrentlyAvailableBandwidth() method from BandwidthEstimatorBase. This means when ABRManager::ResetCurrentlyAvailableBandwidth() is called (which happens during profile rampdown in streamabstraction.cpp), it will invoke the base class's no-op implementation and fail to actually reset the bandwidth estimation state. This could lead to incorrect ABR decisions. Consider adding an override that clears the appropriate internal state (m_history, EWMA values, etc.).

nu641001 requested a review from a team as a code owner January 23, 2026 13:05

Copilot AI review requested due to automatic review settings January 23, 2026 13:05

Copilot started reviewing on behalf of nu641001 January 23, 2026 13:05 View session

Copilot AI reviewed Jan 23, 2026

View reviewed changes

nu641001 force-pushed the feature/VPLAY-12213 branch from a93313d to 8bc0a11 Compare January 23, 2026 15:42

Copilot AI review requested due to automatic review settings January 23, 2026 23:35

nu641001 force-pushed the feature/VPLAY-12213 branch from 8bc0a11 to 98ecf34 Compare January 23, 2026 23:35

Copilot started reviewing on behalf of nu641001 January 23, 2026 23:35 View session

nu641001 force-pushed the feature/VPLAY-12213 branch from 98ecf34 to cdf73de Compare January 23, 2026 23:38

Copilot AI reviewed Jan 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPLAY-12213: ABR Refactoring part 3 - encapsulate remaining abr logic #901

VPLAY-12213: ABR Refactoring part 3 - encapsulate remaining abr logic #901

Uh oh!

nu641001 commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Copilot AI Jan 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

VPLAY-12213: ABR Refactoring part 3 - encapsulate remaining abr logic #901

Are you sure you want to change the base?

VPLAY-12213: ABR Refactoring part 3 - encapsulate remaining abr logic #901

Uh oh!

Conversation

nu641001 commented Jan 23, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants